[jaeger-v2] Add support for GRPC storarge #5228
Conversation
Signed-off-by: James Ryans <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5228 +/- ##
===========================================
+ Coverage 52.95% 94.62% +41.67%
===========================================
Files 149 336 +187
Lines 8796 19521 +10725
===========================================
+ Hits 4658 18472 +13814
+ Misses 3779 861 -2918
+ Partials 359 188 -171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: James Ryans <[email protected]>
plugin/storage/grpc/config/config.go
Outdated
| @@ -78,6 +79,9 @@ func (c *Configuration) Close() error { | |||
| c.pluginHealthCheck.Stop() | |||
| c.pluginHealthCheckDone <- true | |||
| } | |||
| if c.remoteRPCClient != nil { | |||
There was a problem hiding this comment.
To pass the previous code coverage, I set up a gRPC server within the unit test for the factory to use, since we can't just mock the gRPC configuration. Because the current gRPC storage doesn't close the remote gRPC client, I added a method to close the client right inside the factory's Close() function. However, it looks like the code coverage tool isn't recognizing this particular line as covered. Even though I've called the Close() function in my unit test, which should hit this line, the method is abstracted behind an interface. Could this be why it's not showing up as covered?
There was a problem hiding this comment.
in Go coverage is computed in a weird way, it's package local, i.e. coverage is only collected for a package from tests running in the same package. This behavior can be altered by passing additional packages explicitly, as we do for storage integration tests (we tell Go to count coverage in all other packages):
$(GOTEST) -coverpkg=./... -coverprofile cover.out $(STORAGE_PKGS)
So when the integration test runs for grpc-plugin it should include your Close changes, but not if you just run go test manually without -coverpkg
yurishkuro
added
changelog:exprimental
plugin/storage/grpc/factory_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("failed to listen: %v", err) | ||
| } |
There was a problem hiding this comment.
| if err != nil { | |
| t.Fatalf("failed to listen: %v", err) | |
| } | |
| require.NoError(t, err, "failed to listen") |
plugin/storage/grpc/config/config.go
Outdated
| @@ -106,12 +110,13 @@ func (c *Configuration) buildRemote(logger *zap.Logger, tracerProvider trace.Tra | |||
| opts = append(opts, grpc.WithUnaryInterceptor(tenancy.NewClientUnaryInterceptor(tenancyMgr))) | |||
| opts = append(opts, grpc.WithStreamInterceptor(tenancy.NewClientStreamInterceptor(tenancyMgr))) | |||
| } | |||
| conn, err := grpc.DialContext(ctx, c.RemoteServerAddr, opts...) | |||
| var err error | |||
| c.remoteRPCClient, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...) | |||
There was a problem hiding this comment.
this is not a client
| c.remoteRPCClient, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...) | |
| c.remoteConn, err = grpc.DialContext(ctx, c.RemoteServerAddr, opts...) |
plugin/storage/grpc/config/config.go
Outdated
| @@ -78,6 +79,9 @@ func (c *Configuration) Close() error { | |||
| c.pluginHealthCheck.Stop() | |||
| c.pluginHealthCheckDone <- true | |||
| } | |||
| if c.remoteRPCClient != nil { | |||
There was a problem hiding this comment.
in Go coverage is computed in a weird way, it's package local, i.e. coverage is only collected for a package from tests running in the same package. This behavior can be altered by passing additional packages explicitly, as we do for storage integration tests (we tell Go to count coverage in all other packages):
$(GOTEST) -coverpkg=./... -coverprofile cover.out $(STORAGE_PKGS)
So when the integration test runs for grpc-plugin it should include your Close changes, but not if you just run go test manually without -coverpkg
| ) | ||
|
|
||
| // Config has the configuration for jaeger-query, | ||
| type Config struct { | ||
| Memory map[string]memoryCfg.Configuration `mapstructure:"memory"` | ||
| Badger map[string]badgerCfg.NamespaceConfig `mapstructure:"badger"` | ||
| GRPC map[string]grpcCfg.Configuration `mapstructure:"grpc"` |
There was a problem hiding this comment.
I don't think this is how we want to do the configuration in V2, but we can address it more holistically later -- #5229
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
plugin/storage/grpc/factory_test.go
Outdated
| err := f.Close() | ||
| if err != nil { | ||
| log.Fatalf("Client exited with error: %v", err) | ||
| } |
There was a problem hiding this comment.
| err := f.Close() | |
| if err != nil { | |
| log.Fatalf("Client exited with error: %v", err) | |
| } | |
| require.NoError(t, f.Close()) |
| } | ||
|
|
||
| func (s *GRPCStorageIntegrationTestSuite) initialize() error { | ||
| s.logger, _ = testutils.NewLogger() | ||
|
|
||
| if s.factory != nil { |
There was a problem hiding this comment.
how would it be not nil at this point if the factory is only created in L119? Is it from another run of the test? If so we should close it as part of the clean-up, not as part of starting a new test
There was a problem hiding this comment.
Okay, my initial thought was because it restarts the server when initializing, so I think it makes sense if I put it there too. But, moving it to the clean-up process does make things clearer. I've changed it.
Signed-off-by: James Ryans <[email protected]>
Signed-off-by: James Ryans <[email protected]>
|
Do you know why the previous Protogen Validation CI failed? I didn't change anything in the idl module, and now it succeeded without me fixing anything. https://github.com/jaegertracing/jaeger/actions/runs/8066523840 |
|
Unclear why it failed: It's strange that the container doesn't print any more logs about it. |
Which problem is this PR solving?
Description of the changes
How was this change tested?
jaegertracing/jaeger-remote-storageat17271and17281portsgo run -tags=ui ./cmd/jaeger --config ./cmd/jaeger/grpc_config.yamlChecklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test